Test fixes, upgrade quickcheck #640
Conversation
`cargo test --all-targets` excludes doc tests. Experimental features are sometimes mutually exclusive with the stable code.
Overflows chould have distort sound without triggering any errors.
There was a problem hiding this comment.
Nice fixes and improvements 👍 , thank you very much :) . This must have taken a while!
Few things I am unsure about as you'll see in the discussion. Its a bit too late for me to check on the performance impact of the try_from. Ill see if I can add a new interpolate benchmark to rodio tomorrow.
|
Love the work here, can not look at it the next few days unfortunately. After the weekend I'll see if I can come up with a benchmark for the resampler. We need one anyway (I want to introduce a highfy resampler in the future). Ill do a detailed review and answer all the questions that came up then. |
|
Looking a other filters in As I understand rodio in most cases can use generic parameters to avoid dyn references (but Sync and Mixer still use them). That probably can be slightly more efficient. Any other reason rodio cannot use some existing external libraries for sound processing graph? |
Rodio and dasp have fundamentally different goals, that makes it hard and maybe unwise to merge them or require users to be familiar with both.
Rodio predates dasp, so its easy to turn this around and ask, why do they not depend on rodio. Again its a difference in goals, dasp wants to have zero dependencies, rodio just wants to be easy to use (so no C-deps if possible).
I also think rodio has most features dasp has since #602 landed. I might be mistaken there. And a Source that makes it easy to use dasp from rodio might be interesting. The sinc interpolator in dasp is interesting, rodio needs an option for a slower but more hifi interpolator. See #584. |
|
Still have not found the time for the benchmark, as soon as I've done that ill post the results here and we can see what we do based on them. |
|
benchmarks results from main on my machine: If you rebase on (or merge with) main ill rerun them on the PR and we can see if something changed. |
`experimental` flag removes some API, just keep the example minimally functional in that case.
|
benchmarks this pr on my machine: main branch: So that's a 5 to 10% slowdown. A bit too much. Lets see what causes it |
|
with the try from in sample.rs replaced with a cast performance increases a tiny bit however it does not go back to what it was. this pr with changes to sample.rs reverted, again matches main branch in perf: The switch from i32 to i64 is probably to blame. Using i32 might enable the compiler to emit better/any SIMD. We won't know without looking at the assembly. We could keep the |
|
@dvdsk Security-wise there is no strict need in checked cast but u32 * u32 may overflow in some cases. AFAIK overlapped value will wrap over zero in release build, and panic in debug mode. I'd expected some clicks or distortion in case of overflows and panics in debug mode (which was triggered by quickcheck). The final interpolated value should be in range, though. One could use smaller integers for the ratios (I doubt u32 precision is actually necessary there), or use f32 as interpolation coefficient. I think smaller integers, like u16 can be an approximation, maybe even something crude, like dropping some least significant bits in the u32 numerator and divider when one of those has significant bits beyond 16 position. Or finding some other approximate value pair. |
|
@dvdsk Security-wise there is no strict need in checked cast but u32 * u32 may overflow in some cases. AFAIK overlapped value will wrap over zero in release build, and panic in debug mode. I'd expected some clicks or distortion in case of overflows and panics in debug mode (which was triggered by quickcheck). The final interpolated value should be in range, though. One could use smaller integers for the ratios (I doubt u32 precision is actually necessary there), or use f32 as interpolation coefficient. I think smaller integers, like u16 can be an approximation, maybe even something crude, like dropping some least significant bits in the u32 numerator and divider when one of those has significant bits beyond 16 position. Or finding some other approximate value pair. let ratio = num::rational::Rational32::approximate_float(2.3f32).unwrap();
dbg!(ratio.numer(), ratio.denom());Maybe making this more reliable can be a separate task... |
might be related to #584, I thought that was a property of the linear interpolator and could not be fixed. You might have stumbled on the fix for it 🎉
I think that is the best, not only can we try fixing #584 then but we might also look at how to enable users to pick their own re-sampler and introduce more hifi alternatives like rubato or dasps sinc resampler. That should be pretty easy, I think an extra option on the OutputStream builder? On a separate note, how would you feel about joining me as maintainer of rodio? You seem to know your stuff and we can sure use the help. |
|
Yes, I would like to help. I cannot promise much, but I do have some free time for this. |
|
Regarding interpolation, now I think I understand it better. I looks like there are only some specific combinations of conversion rates where it can overflow. I have removed the explicit overflow check from interpolation and tried to clarify the limitations the docs. |
|
Well resampler is just another filter, yes output stream builder can have this option. |
|
CI wants another cargo fmt run 😛. Meanwhile I'll run a quick benchmark and report changes |
|
tldr: Same kind of perf regression as before. Pretty strange, I would expect perf to be the same now I'll hunt for the cause tomorrow. this pr: main: |
| @@ -83,10 +77,13 @@ where | |||
| (first, next) | |||
| }; | |||
|
|
|||
| // Reducing nominator to avoid numeric overflows during interpolation. | |||
| let (to, from) = Ratio::new(to, from).into_raw(); | |||
There was a problem hiding this comment.
I took a look at the Ratio source, and this does the same as the original code that did from: from/gcd and to: to/gcd. See: https://docs.rs/num-rational/latest/src/num_rational/lib.rs.html#108-112. Though I had now idea why they did that until now.
Given that what is the motivation for using Ratio over / gcd?
There was a problem hiding this comment.
It is also used in tests, to limit input values, I thought it makes sense to re-use that and have a bit less code to test. Even if this is an extra call it happens only once at the converter creation.
|
Is there a way to compare the benchmark snapshots? This is the part I liked in |
|
Cargo.lock should be in git somewhere, at least for benches and tests. It is necessary for builds to be reproducible. Projects that have |
It is necessary to make tests and benches reproducible.
quickcheck1.x input values vary more than in 0.9 which finds overflow cases and excessive memory allocation problems inSample::lerp. Calculations now use f64 to avoid that. Conversion back to 16 bit resolution is done explicitly sinceasconversion may silently discard most significant bits. Alternatively, interpolation coefficient (numerator/dividor) may use e.g.u16instead ofu32, or maybe even floating point. I believeu32precision is unnecessary there. But that requires updating sample rate converter which at the moment is the only user of this interpolation.Sample::lerpwas incorrect. It said that calculations should followc * first + (c - 1) * second. which givesfirstatc==1andsecondatc==0but actual implementations use the oppositefirstatc==0andsecondatc==1.no_runsince they require audio devices and may actually make sounds whilecargo testcommand is running.--all-targetsswitch excludes doc tests, so those are executed separately.experimentalbuilds exclude some code and in some code parts use mutually exclusive implementations, so I included those in CI too to keep experimental code compilable.